Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: issue#62 strip auth key #64

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

timazhum
Copy link

Fixing the issue raised in #62

Problem: auth key with leading / trailing whitespaces is throwing exceptions. For example, assigning the secret via echo leaves a trailing \n.

Solution: strip input auth key as an input sanitization procedure

Notice: Java 8 does not support String.strip(), while String.trim() does not support Unicode whitespaces. Therefore, using regex expressions for stripping.

@JanEbbing
Copy link
Member

Oh, this looks very nice, thanks for that! I will review on monday

Copy link
Member

@JanEbbing JanEbbing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fundamentally looks good to me, thanks :)

I'll just get a second opinion from a colleague by tomorrow.

@timazhum
Copy link
Author

Amended the commit, using trim() instead. Also updated a null/empty string checker to match with C# implementation (https://github.com/DeepLcom/deepl-dotnet/blob/main/DeepL/Translator.cs#L438)

@JanEbbing
Copy link
Member

Happy to merge this once Leon's comment is addressed :) thanks for your work!

Fixing the issue raised in DeepLcom#62

Problem: auth key with leading / trailing whitespaces is throwing exceptions. For example, assigning the secret via `echo` leaves a trailing `\n`.

Solution: strip input auth key as an input sanitization procedure

Notice: Java 8 does not support `String.strip()`, while `String.trim()` does not support Unicode whitespaces. We anticipate ASCII only input and utilizing `trim()` for simplicity.
@timazhum
Copy link
Author

Happy to merge this once Leon's comment is addressed :) thanks for your work!

✅ Addressed, thanks for all the feedback provided

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants